Skip to content

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Oct 6, 2025

Fixes a bug which I discovered while using file-based apps in roslyn repo with hardlinks in build opted-in (via ROSLYNUSEHARDLINKS env var).

The File.Copy resulted in "Access denied" exception. I was puzzled at first, but after some investigation discovered that's because the MSBuild task hardlinks obj/app.dll with bin/app.dll and when we then try to File.Copy("obj/app.dll", "bin/app.dll") it fails with that exception because it tries to write to a file which it has locked for reading (because both files are actually the same file via the hardlink).

MSBuild's Copy task doesn't have this problem because it by default checks the files have the same size and timestamp and if so, the copy is skipped. I have implemented the same behavior to fix the issue.

@jjonescz jjonescz added the Area-run-file Items related to the "dotnet run <file>" effort label Oct 6, 2025
@jjonescz jjonescz requested a review from Copilot October 6, 2025 14:39
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug in the file-based app CSC optimization when hardlinks are enabled in the build process. The issue occurred when both source (obj) and destination (bin) files are hardlinked, causing unnecessary copy operations that could fail or behave unexpectedly.

  • Adds a check to compare file size and timestamp before copying compiled assemblies
  • Skips the copy operation when files are already identical (due to hardlinking)
  • Includes a test case to verify the hardlink scenario works correctly

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Cli/dotnet/Commands/Run/CSharpCompilerCommand.cs Implements size and timestamp comparison logic to avoid unnecessary file copies when hardlinks are present
test/dotnet.Tests/CommandTests/Run/RunFileTests.cs Adds test case that enables hardlinks and verifies the optimization works correctly

Comment on lines +113 to +114
var objFile = new FileInfo(parsedArgs.GetOutputFilePath(outputFileName));
var binFile = new FileInfo(BuildResultFile);
Copy link
Preview

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating FileInfo objects here may cause unnecessary file system calls. Consider creating them only when needed inside the HaveMatchingSizeAndTimeStamp method, or check if the files exist first to avoid exceptions.

Copilot uses AI. Check for mistakes.

@jjonescz jjonescz marked this pull request as ready for review October 6, 2025 15:02
@jjonescz jjonescz requested a review from a team October 6, 2025 15:02
@RikkiGibson
Copy link
Member

What was the unwanted behavior, which is being fixed by this PR?

@jjonescz
Copy link
Member Author

jjonescz commented Oct 6, 2025

The File.Copy resulted in "Access denied" exception. I was puzzled at first, but after some investigation discovered that's because the MSBuild task hardlinks obj/app.dll with bin/app.dll and when we then try to File.Copy("obj/app.dll", "bin/app.dll") it fails with that exception because it tries to write to a file which it has locked for reading (because both files are actually the same file via the hardlink).

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider including that longer description of the bug in the final commit message for future context.

@jjonescz jjonescz requested a review from MiYanni October 7, 2025 08:35
@jjonescz jjonescz added this to the 10.0.2xx milestone Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-run-file Items related to the "dotnet run <file>" effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants